-
Notifications
You must be signed in to change notification settings - Fork 34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cached Mementos - To Copy or Not To Copy? #344
Merged
seandenigris
merged 2 commits into
magritte-metamodel:master
from
seandenigris:bug_memento-copying
Jan 14, 2024
Merged
Cached Mementos - To Copy or Not To Copy? #344
seandenigris
merged 2 commits into
magritte-metamodel:master
from
seandenigris:bug_memento-copying
Jan 14, 2024
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Hopefully finally resolves a buffet of related issues (and PRs) mentioned below... - The problem is that mementos were copying described values in two places: - ```smalltalk MACachedMemento>>cookRawPull: aDictionary super cookRawPull: aDictionary. aDictionary keysAndValuesDo: [ :key :value | | isCollectionOfRelations | isCollectionOfRelations := value isCollection and: [ key isKindOf: MAToManyRelationDescription ]. isCollectionOfRelations ifTrue: [ aDictionary at: key put: value copy ] ].``` - There are several problems with the above: Firstly, it is applied to cached mementos. This overly broad, since the motivating problem affects the `original` dictionary - something only checked mementos have. Even worse, it applies to all pulls, not just those to `original` dictionaries. This lead to a host of problems described in more detail below. - ```smalltalk MACheckedMemento>>reset super reset. self setOriginal: (self pullRawTransforming: [ :e | e copy ]). ``` - While this second copying might sometimes be what we want, it clearly does not work in some scenarios, creating the false impression that the model object has changed elsewhere, which prevents committing the memento. - For example, {{gtMethod:MAMementoTest>>testSingletonValue}} was failing because the value was a class and the memento was storing a copying of it. Validation then failed because the installed class is not equivalent to a copy. - Let's take a step back and review the domain model. For checked mementos, there are three versions of model state: - 1. the model itself, which is the real *current* state - 2. the cache, which is the desired state which the memento will attempt to push all at once on commit - 3. the original, which is the memento's copy of the model state at the time the memento was created. This will be used before committing to make sure the model state hasn't changed elsewhere because otherwise committing might intentionally overwrite/destroy needed data. - The motivating problem with the *original*, which inspired all this copying, is that, if a field references a collection, and we hold onto that actual collection, and the elements in the collection are changed from the outside, our "original" will also change and validation will never fail and so offers no protection. - {{gtMethod:MACheckedMementoTest>>testValidateFailsOnReferencedCollectionChange}} tests for this. If we comment out the `#copy` in {{gtMethod:MACheckedMemento>>reset}} it will fail. - Interestingly, outside changes also "bleed" into the cache, but it seems not to matter. If a user was worried about outside changes, they would use an {{gtClass:MACheckedMemento}} which would flag the problem during validation, not an {{gtClass:MACachedMemento}}. Choosing that memento type means we are specifically *not* checking for whether the model has changed elsewhere. Since the cache is changing with the model, there will be nothing to commit unless we explicitly change the field in the memento, which seems like the expected behavior. - There are a bunch of seemingly related GH issues and PRs: - 2019-09-04 - [Mementos Should Ignore Default values](magritte-metamodel#120) - claims to be fixed by the "...Copy Collections" fix below, but I don't see how - 2022-05-31 - [Cached Memento Should Copy ToMany Collections](magritte-metamodel#281) - this first fix led to a lot of future pain. The primary mistake seems to have been that it was applied to all pulls by cached mementos, instead of just pulls to the original dictionary by checked mementos only. The issue talks about "the protection of the cache", but I think I meant "the protection of the original dictionary" because I don't see what damage could be caused by the cache changing underneath you. If you explicitly change a field, it will overwrite the cache, and if not it will be ignored because it is equivalent to the real live model state. - 2022-08-22 - [Checked Memento "Original" Should Copy Collections](magritte-metamodel#295) - the description states "Otherwise changes to the model can bleed through to the "original" dictionary". However, the fix is applied to cached memento, not checked memento only - 2022-08-23 - [Bloc Form To-One Tokens Pointing to Copy](magritte-metamodel#304) - tried to workaround ramifications of the overly broad copy problem fix - 2022-08-23 - [Mementos - Only Copy Checked Original](https://github.com/magritte-metamodel/magritte/pull/305/files) attempted to improve the situation by replacing the original behavior - when pulling copy every value in every memento type - with a more targeted approach - copy only when setting the original of a checked memento. However, this still copied *all* values, not just the problematic referenced collections. It also missed the fact that cached mementos were copying collections during all pulls via `#cookPullRaw:`, not just when pulling to the original dictionary. It also seemingly undid some of the fix above
- Add Lepiter page describing the situation - Remove GT-only method
Fixes #340 |
This was referenced Jan 14, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hopefully finally resolves a buffet of related issues (and PRs) mentioned below...
The problem is that mementos were copying described values in two places:
There are several problems with the above: Firstly, it is applied to cached mementos. This overly broad, since the motivating problem affects the
original
dictionary - something only checked mementos have. Even worse, it applies to all pulls, not just those tooriginal
dictionaries. This lead to a host of problems described in more detail below.While this second copying might sometimes be what we want, it clearly does not work in some scenarios, creating the false impression that the model object has changed elsewhere, which prevents committing the memento.
- For example, {{gtMethod:MAMementoTest>>testSingletonValue}} was failing because the value was a class and the memento was storing a copying of it. Validation then failed because the installed class is not equivalent to a copy.
Let's take a step back and review the domain model. For checked mementos, there are three versions of model state:
1. the model itself, which is the real current state
2. the cache, which is the desired state which the memento will attempt to push all at once on commit
3. the original, which is the memento's copy of the model state at the time the memento was created. This will be used before committing to make sure the model state hasn't changed elsewhere because otherwise committing might intentionally overwrite/destroy needed data.
The motivating problem with the original, which inspired all this copying, is that, if a field references a collection, and we hold onto that actual collection, and the elements in the collection are changed from the outside, our "original" will also change and validation will never fail and so offers no protection.
- {{gtMethod:MACheckedMementoTest>>testValidateFailsOnReferencedCollectionChange}} tests for this. If we comment out the
#copy
in {{gtMethod:MACheckedMemento>>reset}} it will fail.- Interestingly, outside changes also "bleed" into the cache, but it seems not to matter. If a user was worried about outside changes, they would use an {{gtClass:MACheckedMemento}} which would flag the problem during validation, not an {{gtClass:MACachedMemento}}. Choosing that memento type means we are specifically not checking for whether the model has changed elsewhere. Since the cache is changing with the model, there will be nothing to commit unless we explicitly change the field in the memento, which seems like the expected behavior.
There are a bunch of seemingly related GH issues and PRs:
- 2019-09-04 - Mementos Should Ignore Default values - claims to be fixed by the "...Copy Collections" fix below, but I don't see how
- 2022-05-31 - Cached Memento Should Copy ToMany Collections - this first fix led to a lot of future pain. The primary mistake seems to have been that it was applied to all pulls by cached mementos, instead of just pulls to the original dictionary by checked mementos only. The issue talks about "the protection of the cache", but I think I meant "the protection of the original dictionary" because I don't see what damage could be caused by the cache changing underneath you. If you explicitly change a field, it will overwrite the cache, and if not it will be ignored because it is equivalent to the real live model state.
- 2022-08-22 - Checked Memento "Original" Should Copy Collections - the description states "Otherwise changes to the model can bleed through to the "original" dictionary". However, the fix is applied to cached memento, not checked memento only
- 2022-08-23 - Bloc Form To-One Tokens Pointing to Copy - tried to workaround ramifications of the overly broad copy problem fix
- 2022-08-23 - Mementos - Only Copy Checked Original attempted to improve the situation by replacing the original behavior - when pulling copy every value in every memento type - with a more targeted approach - copy only when setting the original of a checked memento. However, this still copied all values, not just the problematic referenced collections. It also missed the fact that cached mementos were copying collections during all pulls via
#cookPullRaw:
, not just when pulling to the original dictionary. It also seemingly undid some of the fix above- 2023-11-18 Checked Memento raises errors for Pier components - there is a now-passing test for just this scenario - MAMementoTest>>testSingletonValue